platform/DRMFormat: Don't require a `FormatInfo` for each format. #3463

RAOF · 2024-07-05T04:34:36Z

We're now using DRMFormat in a situation (linux-dmabuf) where:

We don't actually need to care about many the properties of the format, and
We're processing formats submitted by cilents.

This means that the existing “throw an exception if we haven't got a FormatInfo” approach works badly; we will disconnect clients with a Wayland internal error if they submit buffers in a format we haven't explicitly listed, even though it would work if we just treated the format as an opaque identifier.

So, don't do that anymore. Instead, store the fourcc code directly in DRMFormat, only look up the FormatInfo (now DRMFormat::Info) when code asks for it.

Additionally, we log the first time code asks for the DRMFormat::Info for a format we haven't got info for; this way we can fill out the needed formats on an as-noticed basis.

Fixes: #3462

We're now using `DRMFormat` in a situation (linux-dmabuf) where: * We don't actually need to care about many the properties of the format, and * We're processing formats submitted by cilents. This means that the existing “throw an exception if we haven't got a `FormatInfo`” approach works badly; we will disconnect clients with a Wayland internal error if they submit buffers in a format we haven't explicitly listed, even though it *would* work if we just treated the format as an opaque identifier. So, don't do that anymore. Instead, store the fourcc code directly in `DRMFormat` alongside the `FormatInfo` if it exists. Fixes: #3462

codecov · 2024-07-05T04:57:08Z

Codecov Report

Attention: Patch coverage is 30.23256% with 30 lines in your changes missing coverage. Please review.

Project coverage is 76.97%. Comparing base (eec03e0) to head (2d01c22).
Report is 409 commits behind head on main.

Files with missing lines	Patch %	Lines
src/platform/graphics/drm_formats.cpp	39.13%	14 Missing ⚠️
src/platforms/gbm-kms/server/buffer_allocator.cpp	0.00%	13 Missing ⚠️
src/platform/graphics/linux_dmabuf.cpp	0.00%	3 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #3463      +/-   ##
==========================================
- Coverage   77.36%   76.97%   -0.39%     
==========================================
  Files        1076     1081       +5     
  Lines       68911    69384     +473     
==========================================
+ Hits        53310    53407      +97     
- Misses      15601    15977     +376

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

RAOF · 2024-07-05T07:41:42Z

I'm not sure if I like this; particularly because we now don't have any signal for “it would be useful to have information on this format, but we don't have it”.

It might be better to separate DRMFormat and DRMFormat::FormatDetails, and have the latter be an optional<> that may contain the current details. Then, only code that cares about the details can try to access them, and we can error (or, at least, log a warning/error) if those details are unavailable.

This way things which treat `DRMFormat` as entirely opaque don't pay for the extra format information, and we can warn when the things that *do* care about the format details try to look up details we haven't included yet.

RAOF · 2024-07-08T05:11:13Z

It might be better to separate DRMFormat and DRMFormat::FormatDetails, and have the latter be an optional<> that may contain the current details. Then, only code that cares about the details can try to access them, and we can error (or, at least, log a warning/error) if those details are unavailable.

I have now Done This.

mattkae

A minor thing about symbols, but this makes sense to me overall

src/platform/symbols.map

mattkae

A single question that I might be wrong on, but looks good to me 😄

src/platform/symbols.map

AlanGriffiths

Looks mostly sensible. But we can do better with reporting missing format info

AlanGriffiths · 2024-07-10T11:22:50Z

src/platform/graphics/drm_formats.cpp

+    /* TODO: This could fire quite frequently.
+     * Ideally we would have a rate-limited (or once-per-format) logger, but for now, just
+     * throw it in the debug stream.
+     */
+    mir::log_debug("Detailed info for format %s missing; please report so this can be added", name());


There's a lot of TODOs being added. In linux_dmabuf.cpp we say something similar, but in that context we don't log. I think we need a better approach to reporting this. BTW "rate-limiting" doesn't sound right either.

With this approach, I think "debug" logging requesting reporting is addressing the wrong audience. "debug" and "trace" are for those familiar with the code. "Warning" might be better.

AlanGriffiths · 2024-07-10T11:23:11Z

src/platform/graphics/drm_formats.cpp

+     * throw it in the debug stream.
+     */
+    mir::log_debug("Detailed info for format %s missing; please report so this can be added", name());
+    return {};


std::nullopt?

…issing format info

RAOF · 2024-07-12T04:19:42Z

There we go! I have JFDI and resolved the TODOs rather than adding them.

AlanGriffiths

Some nitpicking. Feel free to land anyway

AlanGriffiths · 2024-07-12T08:32:04Z

src/platform/graphics/drm_formats.cpp

+    if (!unknown_formats->contains(fourcc))
+    {
+        mir::log_warning("Detailed info for format %s missing; please report so this can be added", name());
+        unknown_formats->insert(fourcc);


I know this is a micro optimisation but...

Suggested change

if (!unknown_formats->contains(fourcc))

{

mir::log_warning("Detailed info for format %s missing; please report so this can be added", name());

unknown_formats->insert(fourcc);

if (unknown_formats->insert(fourcc).second)

{

mir::log_warning("Detailed info for format %s missing; please report so this can be added", name());

Also, maybe some guidance on where to report?

I've put a link to GitHub issues in the message text. I haven't taken the micro-optimisation, because I think it makes the intent slightly less obvious and it is the micro-est of micro-optimisations.

…orting missing formats

RAOF requested a review from a team as a code owner July 5, 2024 04:34

platform/DRMFormat: Defer format info lookup until requested

6b31e6e

This way things which treat `DRMFormat` as entirely opaque don't pay for the extra format information, and we can warn when the things that *do* care about the format details try to look up details we haven't included yet.

platform/DRMFormat: Log when we're asked for missing format info

202b2b0

mattkae requested changes Jul 8, 2024

View reviewed changes

src/platform/symbols.map Show resolved Hide resolved

Bump mirplatform ABI; we've dropped some DRMFormat symbols

0c244d6

mattkae approved these changes Jul 9, 2024

View reviewed changes

src/platform/symbols.map Show resolved Hide resolved

AlanGriffiths requested changes Jul 10, 2024

View reviewed changes

RAOF added 2 commits July 12, 2024 13:19

platform/DRMFormat: Log *only once per format* when we're asked for m…

e380660

…issing format info

trivial: Be more precise in returning std::nullopt

f3a0b27

AlanGriffiths approved these changes Jul 12, 2024

View reviewed changes

platform/DRMFormat: Point people at the GitHub new issue page for rep…

2d01c22

…orting missing formats

RAOF enabled auto-merge July 15, 2024 05:32

RAOF added this pull request to the merge queue Jul 15, 2024

github-merge-queue bot removed this pull request from the merge queue due to failed status checks Jul 15, 2024

RAOF added this pull request to the merge queue Jul 15, 2024

Merged via the queue into main with commit 538f6b0 Jul 15, 2024
34 of 36 checks passed

RAOF deleted the dont-die-on-unknown-drm-formats branch July 15, 2024 11:48

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

platform/DRMFormat: Don't require a `FormatInfo` for each format. #3463

platform/DRMFormat: Don't require a `FormatInfo` for each format. #3463

RAOF commented Jul 5, 2024 •

edited

Loading

codecov bot commented Jul 5, 2024 •

edited

Loading

RAOF commented Jul 5, 2024

RAOF commented Jul 8, 2024

mattkae left a comment

mattkae left a comment

AlanGriffiths left a comment

AlanGriffiths Jul 10, 2024

AlanGriffiths Jul 10, 2024

RAOF commented Jul 12, 2024

AlanGriffiths left a comment

AlanGriffiths Jul 12, 2024

RAOF Jul 15, 2024

platform/DRMFormat: Don't require a FormatInfo for each format. #3463

platform/DRMFormat: Don't require a FormatInfo for each format. #3463

Conversation

RAOF commented Jul 5, 2024 • edited Loading

codecov bot commented Jul 5, 2024 • edited Loading

Codecov Report

RAOF commented Jul 5, 2024

RAOF commented Jul 8, 2024

mattkae left a comment

Choose a reason for hiding this comment

mattkae left a comment

Choose a reason for hiding this comment

AlanGriffiths left a comment

Choose a reason for hiding this comment

AlanGriffiths Jul 10, 2024

Choose a reason for hiding this comment

AlanGriffiths Jul 10, 2024

Choose a reason for hiding this comment

RAOF commented Jul 12, 2024

AlanGriffiths left a comment

Choose a reason for hiding this comment

AlanGriffiths Jul 12, 2024

Choose a reason for hiding this comment

RAOF Jul 15, 2024

Choose a reason for hiding this comment

platform/DRMFormat: Don't require a `FormatInfo` for each format. #3463

platform/DRMFormat: Don't require a `FormatInfo` for each format. #3463

RAOF commented Jul 5, 2024 •

edited

Loading

codecov bot commented Jul 5, 2024 •

edited

Loading